Skip to content

refactor(core): pre-resolve child imports before V8 module registration#34050

Merged
bartlomieju merged 5 commits into
mainfrom
fix/loader-hooks-blob-bare-specifier
May 14, 2026
Merged

refactor(core): pre-resolve child imports before V8 module registration#34050
bartlomieju merged 5 commits into
mainfrom
fix/loader-hooks-blob-bare-specifier

Conversation

@bartlomieju

@bartlomieju bartlomieju commented May 14, 2026

Copy link
Copy Markdown
Member

When a child import returned ModuleResolveResponse::Async from the loader,
find_imports_in_compiled_module used to discard the future, place a
placeholder URL on the request with needs_resolve = true, and let
RecursiveModuleLoad issue a second resolve later. That doubled every async
resolve round-trip (the first reply was sent to a dropped oneshot::Receiver
and silently lost) and required the synth-placeholder fallback that was
necessary because base.join(spec) fails for cannot-be-a-base referrers
like blob: and data: — the original bug this PR was opened for: lume's
MDX plugin compiling MDX into a blob: module that imports
lume/jsx-runtime, plus the related vite/rollup case where any transitive
module.register() (e.g. tailwindcss) flipped on resolve_active and broke
node:path imports from inside npm packages.

The fix is to pre-resolve child imports before V8 registers the module.
new_module_from_js_source_with_pending compiles via V8 in scope, tries each
child resolve synchronously, and either:

  • registers and returns NewModuleResult::Ready(id) — fast path when no
    async hooks are active, no behaviour change;
  • stashes the resolve futures inside a PendingModule { handle, requests, pending_resolves, … } and returns Pending.

finalize_pending_module awaits the pending futures, patches the URLs into
the request list, and calls create_module_info. Single worker round-trip
per resolve. register_and_recurse now returns
RegisterOutcome::{Done, PendingFinalize}; both run_to_completion (main /
side loads) and drain_dyn_imports (dynamic imports via op_import) drop
the V8 scope on PendingFinalize, await finalize_pending_module, re-enter
scope, and call finalize_after_pending to register and recurse.

ModuleRequest.needs_resolve, the duplicate resolve_async at
recursive_load::register_and_recurse_inner line 495,
ModuleMap::resolve_async, and the synth-placeholder fallback are all gone
— subsumed by the new flow. Placeholder slots on PendingModule.requests
are never observed by anything downstream. Sync call sites (V8
module_resolve_callback, lazy_load_esm_module, do_load_job) keep using
new_module_from_js_source which now errors clearly if a synchronous
context ever encounters an async-resolve loader, instead of papering over it
with a placeholder.

Adds a spec test under
tests/specs/node/module_register_esm/blob_bare_specifier covering the
original lume case: a passthrough module.register() hook, a blob: module
importing a bare specifier resolved via the import map. The hook writes to
stderr when it sees the bare specifier from a blob: parent and the expected
output asserts on it, ruling out a silent bypass regression. The lume MDX
repro and the rollup-with-hooks node:path regression both pass.

Closes bartlomieju/orchid-inbox#57

…ooks

When module.register() flips on resolve_active, every bare-specifier
resolution is routed through the loader-hook bridge as an async response.
deno_core's async-resolve path computes a placeholder URL by joining the
referrer with the raw specifier; for cannot-be-a-base referrers (blob:,
data:) the join fails and surfaces as "Cannot resolve module \"X\":
relative URL with a cannot-be-a-base base". This broke e.g. lume's MDX
plugin, which compiles MDX to a blob: module that statically imports
bare-mapped specifiers like "lume/jsx-runtime".

Bypasses the hook bridge in both CliModuleLoader and EmbeddedModuleLoader
when the referrer is cannot-be-a-base, and adds a synthetic placeholder
fallback in deno_core so any loader that returns Async in this shape no
longer panics.
…alled

Drop the CLI-side bypass: with the synthetic async-resolve placeholder in
place, the hook bridge now drives bare-specifier resolution from
cannot-be-a-base referrers the same way Node does (verified: Node invokes
the user resolve hook for `bare-spec` with parentURL = the data: URL).

Narrow the placeholder branch in map.rs to trigger only when the parsed
referrer is `cannot_be_a_base()`, so other unforeseen join failures still
surface as typed `Cannot resolve module ...` errors.

Expand the spec test so the hook prints to stderr when it sees the bare
specifier from a blob: parent; the expected output asserts on it, ruling
out a silent bypass regression.
Removes the discard-and-redo pattern at `find_imports_in_compiled_module`:
when a child import returned `ModuleResolveResponse::Async`, deno_core used
to throw the future away, place a placeholder URL on the request with
`needs_resolve = true`, and let `RecursiveModuleLoad` issue a second resolve
later. That doubled every async resolve round-trip and required the
synth-placeholder fallback added in #34050.

Now the resolve future is stored alongside the (compiled) module as a
`PendingModule`. `register_and_recurse` returns `RegisterOutcome::Done` when
every child import resolved synchronously and
`RegisterOutcome::PendingFinalize` when any returned `Async`. The driver
loops -- `run_to_completion` for main/side loads and `drain_dyn_imports` for
dynamic imports -- drop the V8 scope, `await ModuleMap::finalize_pending_module`,
then re-enter scope to register the module with its real child URLs and
recurse. Single round-trip per resolve, no placeholders observed downstream.

Drops `ModuleRequest.needs_resolve`, the duplicate async-resolve in
`recursive_load::register_and_recurse_inner`, `ModuleMap::resolve_async`, and
the `synth_async_resolve_placeholder` added in #34050 -- all subsumed by the
new flow. Sync call sites (V8 `resolve_callback`, `lazy_load_esm_module`,
`do_load_job`) continue to use `new_module_from_js_source`, which now errors
explicitly if a sync context ever hits an async-resolve loader instead of
quietly producing a placeholder. The lume MDX repro and the rollup+hooks
regression both pass.
@bartlomieju bartlomieju changed the title fix(ext/node): bare specifier from blob:/data: referrer with active hooks refactor(core): pre-resolve child imports before V8 module registration May 14, 2026
…-bare-specifier

# Conflicts:
#	libs/core/modules/recursive_load.rs
#	tests/specs/node/module_register_esm/__test__.jsonc
@bartlomieju bartlomieju merged commit 98ae5fc into main May 14, 2026
136 checks passed
@bartlomieju bartlomieju deleted the fix/loader-hooks-blob-bare-specifier branch May 14, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants